-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RUNTIME][IR] Allow non-nullable ObjectRef, introduce Optional<T>. #5314
Conversation
5fa3274
to
4218304
Compare
We use ObjectRef and their sub-classes extensively throughout our codebase. Each of ObjectRef's sub-classes are nullable, which means they can hold nullptr as their values. While in some places we need nullptr as an alternative value. The implicit support for nullptr in all ObjectRef creates additional burdens for the developer to explicitly check defined in many places of the codebase. Moreover, it is unclear from the API's intentional point of view whether we want a nullable object or not-null version(many cases we want the later). Borrowing existing wisdoms from languages like Rust. We propose to introduce non-nullable ObjectRef, and Optional<T> container that represents a nullable variant. To keep backward compatiblity, we will start by allowing most ObjectRef to be nullable. However, we should start to use Optional<T> as the type in places where we know nullable is a requirement. Gradually, we will move most of the ObjectRef to be non-nullable and use Optional<T> in the nullable cases. Such explicitness in typing can help reduce the potential problems in our codebase overall. Changes in this PR: - Introduce _type_is_nullable attribute to ObjectRef - Introduce Optional<T> - Change String to be non-nullable. - Change the API of function->GetAttr to return Optional<T>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me :-)
I'll review Monday morning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Does it make sense to expose optional to python as well?
@MarisaKirisame we don't have to expose optional to python as we can directly use python's Optional for typing. When the normal object typing is used, i believe they are not nullable, the runtime python object themselves are nullable. |
…pache#5314) * [RUNTIME] Allow non-nullable ObjectRef, introduce Optional<T>. We use ObjectRef and their sub-classes extensively throughout our codebase. Each of ObjectRef's sub-classes are nullable, which means they can hold nullptr as their values. While in some places we need nullptr as an alternative value. The implicit support for nullptr in all ObjectRef creates additional burdens for the developer to explicitly check defined in many places of the codebase. Moreover, it is unclear from the API's intentional point of view whether we want a nullable object or not-null version(many cases we want the later). Borrowing existing wisdoms from languages like Rust. We propose to introduce non-nullable ObjectRef, and Optional<T> container that represents a nullable variant. To keep backward compatiblity, we will start by allowing most ObjectRef to be nullable. However, we should start to use Optional<T> as the type in places where we know nullable is a requirement. Gradually, we will move most of the ObjectRef to be non-nullable and use Optional<T> in the nullable cases. Such explicitness in typing can help reduce the potential problems in our codebase overall. Changes in this PR: - Introduce _type_is_nullable attribute to ObjectRef - Introduce Optional<T> - Change String to be non-nullable. - Change the API of function->GetAttr to return Optional<T> * Address review comments * Upgrade all compiler flags to c++14 * Update as per review comment
…pache#5314) * [RUNTIME] Allow non-nullable ObjectRef, introduce Optional<T>. We use ObjectRef and their sub-classes extensively throughout our codebase. Each of ObjectRef's sub-classes are nullable, which means they can hold nullptr as their values. While in some places we need nullptr as an alternative value. The implicit support for nullptr in all ObjectRef creates additional burdens for the developer to explicitly check defined in many places of the codebase. Moreover, it is unclear from the API's intentional point of view whether we want a nullable object or not-null version(many cases we want the later). Borrowing existing wisdoms from languages like Rust. We propose to introduce non-nullable ObjectRef, and Optional<T> container that represents a nullable variant. To keep backward compatiblity, we will start by allowing most ObjectRef to be nullable. However, we should start to use Optional<T> as the type in places where we know nullable is a requirement. Gradually, we will move most of the ObjectRef to be non-nullable and use Optional<T> in the nullable cases. Such explicitness in typing can help reduce the potential problems in our codebase overall. Changes in this PR: - Introduce _type_is_nullable attribute to ObjectRef - Introduce Optional<T> - Change String to be non-nullable. - Change the API of function->GetAttr to return Optional<T> * Address review comments * Upgrade all compiler flags to c++14 * Update as per review comment
…pache#5314) * [RUNTIME] Allow non-nullable ObjectRef, introduce Optional<T>. We use ObjectRef and their sub-classes extensively throughout our codebase. Each of ObjectRef's sub-classes are nullable, which means they can hold nullptr as their values. While in some places we need nullptr as an alternative value. The implicit support for nullptr in all ObjectRef creates additional burdens for the developer to explicitly check defined in many places of the codebase. Moreover, it is unclear from the API's intentional point of view whether we want a nullable object or not-null version(many cases we want the later). Borrowing existing wisdoms from languages like Rust. We propose to introduce non-nullable ObjectRef, and Optional<T> container that represents a nullable variant. To keep backward compatiblity, we will start by allowing most ObjectRef to be nullable. However, we should start to use Optional<T> as the type in places where we know nullable is a requirement. Gradually, we will move most of the ObjectRef to be non-nullable and use Optional<T> in the nullable cases. Such explicitness in typing can help reduce the potential problems in our codebase overall. Changes in this PR: - Introduce _type_is_nullable attribute to ObjectRef - Introduce Optional<T> - Change String to be non-nullable. - Change the API of function->GetAttr to return Optional<T> * Address review comments * Upgrade all compiler flags to c++14 * Update as per review comment
This parameter is nullable for cases where the else block isn't present. Previously, it was represented as a `Stmt` holding `nullptr`, because `IfThenElse` (apache#3533) predates the `Optional` utility (apache#5314). This commit updates to use `Optional<Stmt>` instead, and updates all usages of `else_case`.
This parameter is nullable for cases where the else block isn't present. Previously, it was represented as a `Stmt` holding `nullptr`, because `IfThenElse` (apache#3533) predates the `Optional` utility (apache#5314). This commit updates to use `Optional<Stmt>` instead, and updates all usages of `else_case`.
This parameter is nullable for cases where the else block isn't present. Previously, it was represented as a `Stmt` holding `nullptr`, because `IfThenElse` (apache#3533) predates the `Optional` utility (apache#5314). This commit updates to use `Optional<Stmt>` instead, and updates all usages of `else_case`.
We use ObjectRef and their sub-classes extensively throughout our codebase.
Each of ObjectRef's sub-classes are nullable, which means they can hold nullptr
as their values.
While in some places we need nullptr as an alternative value. The implicit support
for nullptr in all ObjectRef creates additional burdens for the developer
to explicitly check defined in many places of the codebase.
Moreover, it is unclear from the API's intentional point of view whether
we want a nullable object or not-null version(many cases we want the later).
Borrowing existing wisdoms from languages like Rust. We propose to
introduce non-nullable ObjectRef, and Optional container that
represents a nullable variant.
To keep backward compatiblity, we will start by allowing most ObjectRef to be nullable.
However, we should start to use Optional as the type in places where
we know nullable is a requirement. Gradually, we will move most of the ObjectRef
to be non-nullable and use Optional in the nullable cases.
Such explicitness in typing can help reduce the potential problems
in our codebase overall.
Changes in this PR: